fix(swipe): localize the base-layer label in the Layer Swipe panel (#860)#862
Conversation
The Layer Swipe panel relabeled its grouped base layer with a hardcoded
English "Background", so a non-English locale showed "Background" in the
panel while the main layer manager showed the translated label.
Publish the translated base-layer label through the existing layer-display-name
bridge: the controller carries a `backgroundLabel` (set by DesktopShell from
`t("layers.background")` on language change) and includes a `__basemap__`
entry in `__GEOLIBRE_LAYER_LABELS__`. The swipe panel now reads that key,
falling back to "Background" until it is published.
Fixes #860
✅ Deploy Preview for geolibre-app ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR threads a translated background label from ChangesBasemap label localization
Sequence Diagram(s)sequenceDiagram
participant DesktopShell
participant MapController
participant LayerLabels as window.__GEOLIBRE_LAYER_LABELS__
participant syncSwipeLayerLabels
DesktopShell->>MapController: setBackgroundLabel(t("layers.background"))
MapController->>LayerLabels: publishLayerDisplayNames(...)
MapController->>LayerLabels: clearLayerDisplayNames()
syncSwipeLayerLabels->>LayerLabels: read "__basemap__"
syncSwipeLayerLabels->>syncSwipeLayerLabels: use displayName in swipe title
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
⚡ Cloudflare Pages preview
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Code reviewOverall the change is well-structured and closely mirrors the existing BugsNone found. SecurityNone found. PerformanceNone found. Quality
CLAUDE.mdThe new |
- map-controller: publish the synthetic "__basemap__" label after the layer entries so it always wins an id collision (Object.fromEntries keeps the last occurrence), instead of being silently overridden by a layer sharing the id. - map-controller test: reword the misleading comment that implied the class field default was covered; it tests an explicit English push, not the initializer.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Code reviewBugs
Quality
What was checked
|
- map-controller: restore the clear-on-destroy semantic. publishLayerDisplayNames now always emits the basemap entry (needed even with no overlay layers), so destroy() uses a dedicated clearLayerDisplayNames() that empties the bridge instead of leaving the last "__basemap__" label behind. (A layers.length guard would wrongly drop the basemap label in the common zero-overlay case.) - map-controller test: track dispatched events in the window stub and assert the "geolibre-layer-labels-change" event fires on each publish, so a lost dispatch would fail the test; add a test that destroy() clears the bridge.
| // and fires the event again so the panel re-syncs. | ||
| controller.setBackgroundLabel("Hintergrund"); | ||
| assert.equal(win.__GEOLIBRE_LAYER_LABELS__?.__basemap__, "Hintergrund"); | ||
| assert.equal(dispatched.length, 2); |
There was a problem hiding this comment.
Minor test-quality nit: the second dispatch assertion only checks the count but not the event name. The first check (assert.deepEqual(dispatched, ["geolibre-layer-labels-change"])) is strict; the second should be equally strict:
| assert.equal(dispatched.length, 2); | |
| assert.deepEqual(dispatched, [ | |
| "geolibre-layer-labels-change", | |
| "geolibre-layer-labels-change", | |
| ]); |
Also: the companion "clears published labels on destroy" test (withStubbedLabelWindow((win) => …)) ignores the dispatched array entirely, so it does not verify that clearLayerDisplayNames() fires (or does not fire) the change event on teardown. Consider threading in dispatched there too and asserting dispatched.length === 2 (one for setBackgroundLabel, one for destroy).
| private clearLayerDisplayNames(): void { | ||
| if (typeof window === "undefined") return; | ||
| (window as GeoLibreLayerLabelWindow).__GEOLIBRE_LAYER_LABELS__ = {}; | ||
| window.dispatchEvent(new CustomEvent("geolibre-layer-labels-change")); |
There was a problem hiding this comment.
Dispatching geolibre-layer-labels-change on teardown is deliberate (keeps the bridge consistent with the swipe panel if it is still visible), but it has a mild side-effect: the swipe panel's scheduleEnhanceSwipePanel listener fires a requestAnimationFrame callback after the controller is destroyed. The RAF runs harmlessly—document.querySelectorAll returns an empty NodeList when the panel is gone—but if the panel is still mounted at teardown time, it will briefly flash the fallback "Background" label before the new controller's first publish restores the translated value.
This matters only in the reinitialisation path (destroy → new controller created → mapReadyGeneration increments → setBackgroundLabel fires again). The RAF coalescing in scheduleEnhanceSwipePanel (if (swipeEnhanceFrame !== null) return) means the second publish's event typically wins if the two events land in the same frame. In practice the gap is sub-frame, so no visible flicker is expected. Just something to be aware of if a future test asserts on intermediate label state.
No code change needed—documenting the trade-off.
| */ | ||
| setBackgroundLabel(label: string): void { | ||
| this.backgroundLabel = label; | ||
| this.publishLayerDisplayNames(this.syncedLayers); |
There was a problem hiding this comment.
One heads-up: setBackgroundLabel always calls publishLayerDisplayNames(this.syncedLayers), which iterates every synced layer and calls getNamedStyleLayers per layer. This is the same work that syncLayers does on every render, so it is not a new hot path—but it means a single label-push is O(n) in the layer count. If setBackgroundLabel were ever called in a tight loop (it currently is not—only on language-change or controller re-init), this could be noticeable with a large layer list. Not an issue today; just worth knowing if the call site ever changes.
Code reviewOverall this is a clean, well-scoped fix. The approach mirrors the existing BugsNone found. SecurityNone found. No user-controlled input enters the bridge; the label comes from Performance
Quality
CLAUDE.md
|
1 participant
Summary
Fixes #860. The Layer Swipe panel relabeled its grouped base layer with a hardcoded English
"Background"(inswipe-style.ts, which runs outside the React tree and cannot callt()). In a non-English locale the panel showedBackgroundwhile the main layer manager showed the translated label.Changes
Route the translated label through the existing
__GEOLIBRE_LAYER_LABELS__/geolibre-layer-labels-changebridge, mirroring the existingsetCompassLabelpattern for outside-React control strings:packages/map/src/map-controller.ts: add abackgroundLabelfield +setBackgroundLabel(), and publish a__basemap__entry inpublishLayerDisplayNames.DesktopShell.tsx: pusht("layers.background")to the controller on language change / controller re-init (sibling to the existing compass-label effect).swipe-style.ts: read the__basemap__label from the bridge, falling back to"Background"until it is published.Testing
layers.backgroundtranslation: the swipe panel showedBackgroundwhile the sidebar showedHintergrund.Hintergrundin German andBackgroundin English, matching the sidebar. Verified in both light and dark themes (the temporary translation was reverted, so no locale catalog changes ship here).map-controllerunit test asserting the__basemap__label is published and updates on a label change.npm run buildpass; pre-commit (eslint + npm build) clean.Note: the separate on-map MapLibre LayerControl shows its own hardcoded
Background; that is a different surface and out of scope here.Summary by CodeRabbit